-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added a helper function for bc related warp functions #98
base: main
Are you sure you want to change the base?
Conversation
Please add the helpers to the init and import them from xlb.helper similar to the other helper functions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above
It is not possible because the DefaultConfig is not set before xlb.init is called! |
Then this is not ideal. The import of helper functions should not depend on calling xlb.init(). Please find a better solution or consider just consolidating these functions in the BCs. |
Not sure why this is an issue here. This helper function does not call xlb.init() and it cannot be used anywhere ouside of BC implementations where xlb.init() has already been called. Having everything inside init is also not a requirement for packaging. Can you give a scenario where the current implementation might cause an issue? |
A "helper" function should not rely on initialization and must be usable independently. Since these functions may be invoked by users in examples, please relocate them to the boundary condition base class, as their functionality aligns more closely with that context. |
Moving them to the base class is exteremely messy because they have to be added one by one inside class Init method and given to self. The Warp functions need to be enclosed within a python function to declare the needed constants and variables that are out of the scope of that wp.func! I think the current approach is the cleanest. Please recommend a better approach and I will gladly change it. |
There are few methods right now inside the bc base class that are defined inside the init. I should perhaps move them to this helper function too and import them whenever needed. That would be for this PR. If you agree I can do that as well. |
…ical warp functions.
f2db380
to
00b678b
Compare
I moved the new file from helper folder to BC folder which I think addresses the comment about "helper" function. |
Added a helper function for BCs to avoid repeated definition of identical warp functions.
Contributing Guidelines
Description
We cannot inherit methods in Warp. As a workaround, rather than repeating snippets to represent identical Warp functionalities (which is what is done currently in the code), this PR creates a helper function that keeps those methods. These methods are imported as needed inside construct_warp.
Type of change
How Has This Been Tested?
Linting and Code Formatting
Make sure the code follows the project's linting and formatting standards. This project uses Ruff for linting.
To run Ruff, execute the following command from the root of the repository:
ruff check .